-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deere 2.1 part 2 #1343
Deere 2.1 part 2 #1343
Conversation
Good Idea! |
res/skins/Deere/tool_bar.xml
Outdated
</Template> | ||
|
||
<Template src="skin:knob_toolbar.xml"> | ||
<SetVariable name="TooltipId">master_gain</SetVariable> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong tooltip
res/skins/Deere/tool_bar.xml
Outdated
<SetVariable name="group">[Master]</SetVariable> | ||
<SetVariable name="control">booth_gain</SetVariable> | ||
<SetVariable name="color">red</SetVariable> | ||
<SetVariable name="label">Booth</SetVariable> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we have Gain / Head Gain / Booth .. three naming schemas for similar features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm... haven't checked it at all, but here is my opinion:
"Gain", without any other label, should be used where the context limits it to a specific control. I.e. track gain, sampler gain, Master gain should tell that it is the Master.
"Head gain": if not using a hardware control (present in many controllers) the UI specific control for headphones should be labeled this way.
"Booth gain": AFAIK, the booth is the speakers/monitors that the DJ can have inside the cabin to listen to the audio. They are of importance when the cabin's position can't give the DJ enough detail. It can serve two purposes: to better equalize, or to better mix (getting a cleaner source of what's being played while also listening to the headphones). Some other times it might simply be for DJ amusement :P
Anyway, booth gain does not have much sense on Mixxx since we don't have an explicit Booth output and it should not be confused with headphones output. Booth is generally connected to the secondary output of a Mixer table, containing also the master output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that "GAIN" without any context is not clear. I have changed it to "MASTER".
The booth output was introduced in #1279. The gain knob for it is only shown if a booth output is configured. It is useful so a sound card with sufficient output channels can be plugged directly into the main and booth speakers without the signal getting degraded by going through an unnecessary mixer.
and add tooltip for booth gain
Users were getting confused by the bottom splitter and having two spltiters in the same direction made resizing them wonky. The only purpose of the bottom splitter was to hide the EQ knob labels, so just remove the EQ knob labels always.
There's some weird asymmetry going on in the mixer. Not sure why...
some time ago, we decided for the word "gain" to state clear that this is a digital pre-amplifier gain with a bitcrusher nature, useful to level the output for maximum signal noise ratio. If we drop the word gain, the issue is back. Can you add a Group label "Gain" for them? Maybe it is time to work on this bug, to finally fix this annoying and long standing issue: |
Whether it's labeled "MASTER" or "GAIN" in the skin is irrelevant to that issue IMO. Neither makes it clear that the knob does not affect the hardware volume control, nor do I think there is any succinct label that could make that clear. I understand your desire to try to implement proper volume control in an easy way that users wouldn't be likely to mess up, I do not think it is possible with the heterogeneity of operating systems and sound cards out there. Many sound cards do not even expose the hardware volume control to the computer. Even if they did, I think it would be futile to try to manipulate the hardware volume control reliably across each OS and all the complications of the various sound APIs on Windows. IMO the best we can do is document what the software gains do and advise not to use them in most cases.
Can we please focus on finishing what we have started and getting a release out? |
The refactoring to get rid of the bottom splitter is done, but there is now some strange asymmetry between the left and right sides of the mixer I am trying to resolve. It is most apparent by right clicking the crossfader to center it and seeing that it does not align with the master level meter. |
We had a condense about the GAIN label. An it is an issue for me that the controls are looking like volume in Tango. |
We can IMHO release at any time. If we wait for all started things to be finished (like we did now for more than year) we will get never a release. The today master is IMHO stable enough to be a beta. |
I am thinking about removing the "Minimal Controls" mode. I implemented that as a quick hack around the clutter of the old looping UI. Now that all controls fit into a reasonable area, I think it can be removed. I have found myself frustrated and confused a number of times when I had the Minimal Controls mode active and wanted access to the hidden controls. Thoughts? |
This reduces shutdown time by about 1 second for me.
That would be Ok for me. We can advertise Deere as a full feature skin. Tango and Schade can be focused on other use cases. An additional Broadcasting skin would be real great |
Microphones can't be assigned to the crossfader in the engine.
This was a quick hack around the clutter of the old looping UI. Now that all controls fit into a reasonable amount of space and the bottom splitter has been removed, get rid of this option. On many occasions I found myself confused why I couldn't access a control in minimal controls mode then annoyed that I had to open the skin settings menu to get to what I wanted.
TODO: Make a new knob widget to show the proper tooltip
966a3b3
to
d8fccba
Compare
I added an option to hide the EQ kill switches. Few controllers have buttons for those so they waste space for most users. However, they're useful for mouse & keyboard users. |
Like @sblaisot I miss sampler gain. A knob in the top bar might be sufficicent, visible only when samplers are enabled. |
I'm not pushing any more commits to this PR until I get write access. This is ridiculous that this branch has been sitting ready to merge for 1.5 months and pull requests are being targeted at my fork instead of master. The discussion has strayed far off topic from the original purpose of this PR. |
Sorry for not following all details of this branch an merge it in a time. @ronso0: Is it merge-able in the current state? Do we have a finished review with a formal LGTM yet? @Be-ing: There is still an open TODO in the description, do you plan to fix it? I do not understand how master write access will change the situation? We have discussed that no one should merge his own PRs without a formal LGTM. Thats why #894 is for example also not merged. Accepting PRs to private branches is IMHO a good technique to push a branch forward, that is not in a mergable state. |
It's not your fault. It should not be any one person's responsibility to follow every pull request in detail.
Yes, I do. The master level meter with 4 decks parallel waveforms is not the same height as the deck meters, but I don't think that's necessary to fix before merging. We could go on forever improving this but it has been in a good mergeable state that is better than master for weeks. For the future let's separate TODO lists into tasks that must be fixed before merge and those that don't need to be fixed before merge so this is clearer.
I agree that we generally shouldn't merge our own PRs. @ronso0, @uklotzde, and @sblaisot have tested this and should have been able to merge it too. PRs for private branches are good for branches that aren't in condition to merge to master, but this has been for a while. These absurdly long delays have been happening repeatedly, especially for skin PRs, and all it does is frustrate users and make it more annoying to do development. |
@Be-ing A while ago you asked me to help you fix Deere because you expexted to have little time for it. |
I think for skin PRs like this we should generally make sure that they were tested on all platforms before merge, maybe by checkboxes in the OP. See previous glitches in EffectSelector and beat spinboxes. |
@ronso0, you've done nothing wrong by sending me PRs and I apologize if it came off that way. My point is this branch should've been merged to master weeks ago and your recent PRs should have been able to be targeted at master. |
That would be ideal, but I don't think we have enough people testing on every platform (especially macOS) to make that a requirement for merging a PR. It should certainly be a requirement for a release though. |
IMO this is ready to merge. On Linux it's fine, @sblaisot tested it for Windows I suppose. Did anyone do so for OSx? I'll re-open the knob redesign PR against master edit: as soon as this merged (easier than rebasing). |
I did not explicitly ask for merging this before because I am exhausted from having to repeatedly ask one person for every little change. This cannot be one person's responsibility if we are going to have releases more often than every two years. We have "continuous integration" services, but what we practice is the opposite of continuous integration. |
Okay, I somehow understand your frustration and it's great you started that email discussion about it. |
@esbrandt Are you able to take a look at this? |
Deere 2.1 layout & color fixes
@Be-ing There is still one open task on your check list. Do you plan to finish it here? Otherwise I would like to merge this PR and the polishing can be done afterwards. We already have #1399 as a follow up. Great job 👍 The UI of Mixxx is evolving at an impressive speed. I hope we can pick up this momentum for other parts of the application, too! We definitely need more domain experts in all areas. |
@uklotzde I fixed the mixer layout for all configurations.
I think this is ready to merge, too. |
The channel meters are tiny in the full featured 4-deck parallel layout, anyway. Having a bigger master level meter instead of wasting space might be desirable, even if it doesn't match the scaling of the channel meters. We might find a compromise or even new ideas for different options afterwards. LGTM. |
Merging this now since no one voted against it. The beta release date is near and we should get feedback from some alpha testers about our main skin. |
Pull requests welcome. |
I didn't make the new pull request to fix the square boxes because of the
other problem with the samplers that I don't know how to fix. Anyway, I'll
repeat here what's the problem: the unicode characters used in the skin for
these graphics does not exist in the Windows fonts. There are other codes
with similar characters that are present on both, Linux and Windows, but I
don't have it here right now.
|
Well. the problem is bigger than that. We explicitely load a font. so we shouldn't rely on fallback font using characters that are not embedded in these fonts. for me, we have 2 solutions:
|
I know that :) |
I found the email I sent to be. As for loading a font, is Mixxx embedding
fonts? Afaik, it only uses installed fonts.
I finally found out what happens, which is that the characters that you
use, are not recognized on Windows.
I found replacements that work on both, Windows and Linux (I even tried
under Windows 7):
Here, the ones you use (in HEX)
https://www.fileformat.info/info/unicode/char/26AA/index.htm
https://www.fileformat.info/info/unicode/char/26AB/index.htm
Here, the compatible ones (in HEX)
https://www.fileformat.info/info/unicode/char/25CB/index.htm
https://www.fileformat.info/info/unicode/char/25CF/index.htm
Decimal values correspond to 9675 and 9679
|
Yeah, that works ! Thanks. PR #1430 |
Continuing work on the Deere skin. Now that I have been using it for a while, I am focusing on simplifying it in this PR.
I will be refactoring it to remove the bottom splitter that has been confusing users. Having two splitters in the same direction makes resizing them wonky at times. The only purpose of that was to make it possible to hide the EQ knob labels and have the mixer take less vertical space. Instead of making that optional, I'll just remove the EQ knob labels. LateNight and Tango already do not have the EQ knobs labeled. This may be a little bit confusing for new users completely unfamiliar with DJ equipment, but so are many other parts of the GUI and tooltips are good enough for those. To anyone who has ever used any DJ gear, it shouldn't be an issue at all.
TODO: